fix: Fix TypeError in Slice.get() method when using filter_by() with BinaryExpression #34769

sadpandajoe · 2025-08-19T23:35:07Z

SUMMARY

Problem

The Slice.get() method introduced in PR #29573 was causing a TypeError that broke chart thumbnail generation.

TypeError: Query.filter_by() takes 1 positional argument but 2 were given

Root Cause Analysis

The Bug

In superset/models/slice.py line 366, the code incorrectly used filter_by() with a BinaryExpression:

  # BROKEN
  qry = db.session.query(Slice).filter_by(id_or_uuid_filter(id_or_uuid))

Why It Failed

filter_by() expects keyword arguments: filter_by(id=123, name="test")
id_or_uuid_filter() returns a BinaryExpression object like Slice.id == 123
SQLAlchemy requirement: BinaryExpression objects must use filter(), not filter_by()

Pattern Comparison

The bug was introduced when copying the UUID support pattern from Dashboard.get(), but using the wrong filter method:

  # CORRECT (Dashboard.get() - already working)
  qry = db.session.query(Dashboard).filter(id_or_slug_filter(id_or_slug))

  # INCORRECT (Slice.get() - what was broken)  
  qry = db.session.query(Slice).filter_by(id_or_uuid_filter(id_or_uuid))

Solution

One-line fix: Change filter_by() to filter() in Slice.get()

  # FIXED
  qry = db.session.query(Slice).filter(id_or_uuid_filter(id_or_uuid))

Impact & Testing

What This Fixes

✅ Chart thumbnail generation - cache_chart_thumbnail() in superset/tasks/thumbnails.py
✅ All Slice.get() calls - Both ID and UUID string lookups now work
✅ API endpoints - Any chart API calls that trigger Slice.get()

Comprehensive Test Coverage Added

Unit Tests (tests/unit_tests/models/slice_test.py - 5 tests):

Mock verification that filter() is called instead of filter_by()
Integration tests with real database calls for ID and UUID scenarios
Helper function tests for id_or_uuid_filter()

Integration Tests (tests/integration_tests/charts/api_tests.py - 4 tests):

test_slice_get_by_id() - Test with numeric ID strings
test_slice_get_by_uuid() - Test with UUID strings
test_slice_get_nonexistent_id() - Test error handling for invalid IDs
test_slice_get_nonexistent_uuid() - Test error handling for invalid UUIDs

Investigation Depth

✅ Confirmed isolation: Only Slice.get() was broken, Dashboard.get() was already correct
✅ No other models affected: Only charts/slices use this pattern
✅ Screenshots analysis: Dashboard screenshots work (correct implementation), only chart screenshots were broken
✅ Downloads unaffected: Export/download functionality uses different code paths

🤖 Generated with Claude Code

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: fixes: Screenshots cannot be generated with 6.0.0rc1 #34832
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

Fix TypeError in Slice.get() method introduced in PR #29573. The method was incorrectly using filter_by() with a BinaryExpression from id_or_uuid_filter(), which expects keyword arguments. Changed to use filter() which accepts BinaryExpressions. This fixes: - Chart thumbnail generation failures - 9 failing tests (8 event logger, 1 multi-tenant) - All calls to Slice.get() with ID or UUID strings Added comprehensive test coverage: - Unit tests for Slice.get() method with mock verification - Integration tests for both ID and UUID lookup scenarios - Tests to prevent regression of filter_by usage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

korbit-ai

I've completed my review and didn't find any issues.

Files scanned

File Path	Reviewed
superset/models/slice.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

Fixed integration tests that were incorrectly passing UUID objects instead of string representations to Slice.get() method: 1. test_slice_get_by_uuid: Changed chart.uuid to str(chart.uuid) 2. test_slice_get_nonexistent_uuid: Changed invalid UUID string "non-existent-uuid-123" to proper UUID format using uuid.uuid4() The id_or_uuid_filter function was correctly designed to accept string inputs (id_or_uuid: str), but our tests were passing wrong data types. This fixes the GitHub Actions CI failures. Addresses issues identified in PR #34769 CI logs. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

Co-authored-by: Copilot <[email protected]>

tests/unit_tests/models/slice_test.py

Copilot

Pull Request Overview

This PR fixes a TypeError in the Slice.get() method that was preventing chart thumbnail generation and breaking API calls. The bug was caused by incorrectly using filter_by() with a BinaryExpression when filter() should have been used.

Fixed the SQLAlchemy query method from filter_by() to filter() in Slice.get()
Added comprehensive unit tests to verify the fix and prevent regression
Added integration tests to ensure real database scenarios work correctly

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
superset/models/slice.py	One-line fix changing filter_by() to filter() in Slice.get() method
tests/unit_tests/models/slice_test.py	New unit test file with 5 tests covering mock verification and helper function testing
tests/integration_tests/charts/api_tests.py	Added 4 integration tests for real database scenarios with ID/UUID lookups

Copilot · 2025-08-20T06:12:34Z

tests/unit_tests/models/slice_test.py

+        for test_input in test_cases:
+            try:
+                result = Slice.get(test_input)
+                # Success - no TypeError occurred, result can be None or a Slice.  # noqa: E501


[nitpick] The comment formatting is inconsistent with the period placement before the noqa comment. Consider moving the period after the noqa comment or removing it entirely for better readability.

Suggested change

# Success - no TypeError occurred, result can be None or a Slice. # noqa: E501

# Success - no TypeError occurred, result can be None or a Slice # noqa: E501.

The # no qa is not part of the comment instead it's so we don't get an error saying the length is too long.

Copilot · 2025-08-20T06:12:34Z

tests/unit_tests/models/slice_test.py

+        # Should return a BinaryExpression that can be used with filter()
+        assert result is not None
+        # The important thing is it doesn't crash and returns a filter expression.  # noqa: E501
+


[nitpick] The same comment is duplicated in both test methods. Consider extracting this into a shared docstring or making the comments more specific to each test case.

Suggested change

Why would we remove a line break here. That would leave 0 breaks between this and the next test.

Copilot · 2025-08-20T06:12:34Z

tests/unit_tests/models/slice_test.py

+        result = id_or_uuid_filter(test_uuid)
+        # Should return a BinaryExpression that can be used with filter()
+        assert result is not None
+        # The important thing is it doesn't crash and returns a filter expression.  # noqa: E501


[nitpick] The same comment is duplicated in both test methods. Consider extracting this into a shared docstring or making the comments more specific to each test case.

Suggested change

# The important thing is it doesn't crash and returns a filter expression. # noqa: E501

Antonio-RiveroMartnez · 2025-08-20T06:18:30Z

tests/integration_tests/charts/api_tests.py

        rv = self.get_assert_metric(uri, "get")
        assert rv.status_code == 404

+    def test_slice_get_by_id(self):


Could we merge these tests together and avoid duplication? both for the success and non existent ones

Are you saying combine the id ones together and the uuid ones together? I normally prefer them separate in case if one use case fails and the other one passes, but can see that the uuid one might not actually be testing anything if we aren't in the if statement clause.

Yes, I mean, looking at the tests you can assert both things just with 1 extra assertion that's why I suggested merging them, but the ultimate goal is to DRY them because those tests are 95% the same

Antonio-RiveroMartnez · 2025-08-20T06:19:23Z

tests/unit_tests/models/slice_test.py

+from superset.models.slice import id_or_uuid_filter, Slice
+
+
+class TestSlice:


Same comment from above

@Antonio-RiveroMartnez good call. Parameterized it so I'd still know if one of the use cases failed but no duplicated code.

Address PR reviewer feedback by consolidating duplicate test cases into parameterized tests while maintaining test isolation and clarity. Changes: - Unit tests: 5 separate tests → 3 parameterized tests (8 test cases) - Integration tests: 4 separate tests → 2 parameterized tests (4 test cases) - Fixed UUID no-op issue: Use pytest.skip() if chart.uuid is None - Added uuid import to integration tests Benefits: - Reduced code duplication significantly - Each parameter still runs as separate test case - Clear failure identification with descriptive test names - Easier to add new test scenarios - Better maintainability Test coverage remains comprehensive: - Mock-based contract testing for filter() vs filter_by() - Integration testing with real database calls - Error handling for non-existent IDs/UUIDs - BinaryExpression testing for id_or_uuid_filter function Addresses feedback from Antonio-RiveroMartnez while preserving the test isolation benefits originally preferred. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

Antonio-RiveroMartnez

lgtm

…BinaryExpression (#34769) Co-authored-by: Claude <[email protected]> Co-authored-by: Copilot <[email protected]> (cherry picked from commit 9de1706)

pull-request-size bot added the size/L label Aug 19, 2025

dosubot bot added api Related to the REST API change:backend Requires changing the backend labels Aug 19, 2025

korbit-ai bot reviewed Aug 19, 2025

View reviewed changes

github-actions bot removed the api Related to the REST API label Aug 20, 2025

sadpandajoe added this to Apache Superset 6.0.X Release Bugs Aug 20, 2025

sadpandajoe moved this to New in Apache Superset 6.0.X Release Bugs Aug 20, 2025

sadpandajoe added the 6.0 label Aug 20, 2025

sadpandajoe requested review from Copilot and michael-s-molina August 20, 2025 06:08

This comment was marked as outdated.

Sign in to view

sadpandajoe and others added 2 commits August 19, 2025 23:10

Update tests/unit_tests/models/slice_test.py

1a9997a

Co-authored-by: Copilot <[email protected]>

Update tests/unit_tests/models/slice_test.py

f55041a

Co-authored-by: Copilot <[email protected]>

sadpandajoe commented Aug 20, 2025

View reviewed changes

tests/unit_tests/models/slice_test.py Outdated Show resolved Hide resolved

Update tests/unit_tests/models/slice_test.py

348d836

sadpandajoe requested a review from Copilot August 20, 2025 06:11

Copilot AI reviewed Aug 20, 2025

View reviewed changes

Antonio-RiveroMartnez reviewed Aug 20, 2025

View reviewed changes

rusackas added the cares:preset Preset cares about this issue label Aug 20, 2025

rusackas requested a review from Antonio-RiveroMartnez August 20, 2025 17:24

Antonio-RiveroMartnez approved these changes Aug 20, 2025

View reviewed changes

sadpandajoe merged commit 9de1706 into master Aug 20, 2025
55 checks passed

sadpandajoe deleted the fix/slice-get-filter-by-bug branch August 20, 2025 18:22

sadpandajoe added the 6.0:checkpoint label Aug 20, 2025

sadpandajoe moved this from New to Cherried in Apache Superset 6.0.X Release Bugs Aug 20, 2025

sadpandajoe removed the 6.0:checkpoint label Aug 21, 2025

dosubot bot mentioned this pull request Aug 23, 2025

Screenshots cannot be generated with 6.0.0rc1 #34832

Closed

3 tasks

sadpandajoe added v6.0 Label added by the release manager to track PRs to be included in the 6.0 branch and removed 6.0 labels Aug 26, 2025

github-actions bot added 🍒 6.0.0 Cherry-picked to 6.0.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Dec 18, 2025

	# Success - no TypeError occurred, result can be None or a Slice. # noqa: E501
	# Success - no TypeError occurred, result can be None or a Slice # noqa: E501.

		from superset.models.slice import id_or_uuid_filter, Slice


		class TestSlice:

fix: Fix TypeError in Slice.get() method when using filter_by() with BinaryExpression #34769

fix: Fix TypeError in Slice.get() method when using filter_by() with BinaryExpression #34769

Conversation

sadpandajoe commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

Problem

Root Cause Analysis

Why It Failed

Pattern Comparison

Solution

Impact & Testing

What This Fixes

Comprehensive Test Coverage Added

Integration Tests (tests/integration_tests/charts/api_tests.py - 4 tests):

Investigation Depth

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

korbit-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Antonio-RiveroMartnez Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Antonio-RiveroMartnez Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Antonio-RiveroMartnez Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sadpandajoe commented Aug 19, 2025 •

edited

Loading